-
Notifications
You must be signed in to change notification settings - Fork 3
[TSD-47] Create SQLite schema and insert data using HTTP #25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
HirotoShioi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Just few changes. I'll take a further look afterwards to see if we can make more improvements.
src/DataSource/Http.hs
Outdated
| , cfgZendesk = "https://iohk.zendesk.com" | ||
| , cfgToken = "" | ||
| , cfgEmail = "daedalus-bug-reports@iohk.io" | ||
| , cfgEmail = "kristijan.saric@iohk.io" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should revert this to daedalus-bug-reports@iohk.io. This address is used when the classifier is posting comments on Zendesk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to move some of these to CLI, this doesn't make sense here.
| :: forall m. (MonadIO m, MonadReader Config m) | ||
| => ExportFromTime | ||
| -> m [TicketInfo] | ||
| getExportedTickets time = do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function seems to be doing a lot of unnecessary printing.. Can you clean it up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't say it's "unnecessary" since I required that in order to run it and find three major issues.
I have an alternative though, will do it in a separate ticket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
| let typedURL = showURL $ TicketCommentsURL ticketId | ||
| untypedURL = "/tickets/" <> show (getTicketId ticketId) <> "/comments.json" | ||
| in typedURL == untypedURL | ||
| it "returns valid ExportDataByTimestamp" $ do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is failing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Speaking of failing tests, did you add these:
-
processes ticket, with comments FAILED [1] -
processes ticket, with no attachments FAILED [2] -
processes ticket, with attachments FAILED [3]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I did, and it's strange because those tests are passing in the develop branch.
HirotoShioi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides failing tests, everything seems good!
https://iohk.myjetbrains.com/youtrack/issue/TSD-47
Run it for almost a day and it skips "phantom" tickets and inserts the data into the database.
After it's in the database, the data is available almost instantly.
There is still plenty of work to be done, one of the priorities is to add exception handling because it's getting very painful to run something just to see it crash a couple of minutes after that.